Skip to content

WIP: try legends on native again - #29361

Open
chrisnojima wants to merge 30 commits into
masterfrom
nojima/HOTPOT-legends-310
Open

WIP: try legends on native again#29361
chrisnojima wants to merge 30 commits into
masterfrom
nojima/HOTPOT-legends-310

Conversation

@chrisnojima

Copy link
Copy Markdown
Contributor

No description provided.

@chrisnojima
chrisnojima force-pushed the nojima/HOTPOT-legends-310 branch 2 times, most recently from c11edb7 to b7612ce Compare July 9, 2026 19:16
@chrisnojima
chrisnojima changed the base branch from master to nojima/HOTPOT-message-fixes July 9, 2026 19:17
@chrisnojima
chrisnojima force-pushed the nojima/HOTPOT-message-fixes branch from a621b5c to 32a520c Compare July 9, 2026 19:23
@chrisnojima
chrisnojima force-pushed the nojima/HOTPOT-legends-310 branch from b7612ce to 5e96b94 Compare July 9, 2026 19:23
Base automatically changed from nojima/HOTPOT-message-fixes to master July 9, 2026 20:34
@chrisnojima
chrisnojima force-pushed the nojima/HOTPOT-legends-310 branch 2 times, most recently from e016702 to 9d9678d Compare July 17, 2026 20:39
…ights

These are list-agnostic row fixes, split out of the native LegendList work.
Desktop already renders the thread with a recycling LegendList, so each of
these is a live bug there today; on the native FlatList they are inert
guards.

- per-row state (sent animation, reaction picker, exploding retained height,
  ash tower) is now keyed to messageKey. A recycled container reuses the
  component instance for a different message, so mount-captured state leaked
  across rows: the picker stayed open on the wrong row, the sent-animation
  wrapper stuck on, and a measured retain height was applied to the wrong
  message (and could not self-correct, since retainHeight forces the style
  height so onLayout only ever reports the forced value back)

- recycle-pool suffixes are limited to ones that are stable for the message's
  lifetime (:failed, :reply). The pool label is recorded when a container is
  allocated and never updated in place, so :pending (flips on every send
  confirmation) and :reactions (toggles) left stale labels behind and recycled
  containers painted at the wrong pooled height

- getItemType splits headered rows into their own pool (:hdr). A message that
  leads its author group is ~40px taller than a grouped follow-on of the same
  render type, so a shared pool paints recycled views at the wrong height for
  a frame. It reads the same sticky username cache the rows render with, else
  a row that keeps its header after a scroll-back load is typed headerless and
  poisons the headerless pool's height average

- useSyncRowLayout: when a row's content settles to a new height after first
  paint (flip result streams in, reactions appear, an unfurl loads), flush the
  row measure synchronously so the list's bottom re-pin uses the final height
  on the same frame instead of a frame late, which otherwise parks the thread
  above the newest message

- SwipeableRow takes an `enabled` prop that conditionally spreads its pan
  handlers, so a list can shed per-row touch evaluation during a fast fling
  without unmounting the row. Toggling the Swipeable's subtree instead would
  remount its children and flash images. No caller passes it yet

- desktop LegendList: dataKey replaces the key remount on conversation switch,
  and maintainScrollAtEnd opts back into footerLayout, which 3.x stopped
  implying once the trigger set is given explicitly
Replaces the inverted FlatList + KeyboardChatScrollView native thread with a
non-inverted KeyboardAwareLegendList, on top of the list-agnostic row fixes in
the parent commit.

- non-inverted list: rows render in natural order, so the sent-message
  animation, separators, and header/footer components swap ends. Bottom
  clearance for the input bar is reserved statically via contentContainerStyle,
  with the keyboard composer inset seeded to 0 so the two don't stack.

- pagination: onStartReached/onEndReached replace the viewability-window
  heuristic (useNativeSafeOnViewableItemsChanged is deleted).

- prepend jump: maintainScrollAtEnd's dataChange trigger re-pins on ANY data
  change within maintainScrollAtEndThreshold of the end, so on short threads a
  load-older prepend yanked the view to the bottom. The threshold stays wide
  (0.5) because initialScrollAtEnd positions from estimatedItemSize and lands
  short on our tall rows; instead the re-pin is suspended while a prepend is in
  flight (prependActive).

- initial position: the list is not mounted until the thread is loaded, so its
  first render always has data and initialScrollAtEnd lands at the newest
  message. Previously, arriving from the inbox mounted the list empty and the
  initial scroll ran against no data and never re-fired.

- centering: scrollToItem(viewPosition: 0.5) lands accurately here, so the
  closed-loop offset corrector (viewable-range feedback, damped item-delta
  scrolls) is deleted in favor of re-asserting across the pagination settle.
  onScrollToIndexFailed goes with it — LegendList has no such prop.

- maintainVisibleContentPosition is on from mount so prepends hold position.

- fling cost: experimental_adaptiveRender lets rows shed their swipe pan
  handlers during fast scroll, via useAdaptiveRender in long-pressable driving
  the SwipeableRow `enabled` prop added in the parent commit. useAdaptiveRender
  reads LegendList's state context and throws outside a LegendList, so it can
  only be wired up here. The Swipeable stays mounted — toggling its tree would
  remount children and flash images.

- stable renderItem: NativeRow reads the centered highlight itself, so
  renderItem identity never changes and a highlight change doesn't re-render
  every visible row.

Includes a temporary [LISTDBG] dump for the initial-load settle; remove before
merging.
…ssal

The keyboard-hide contentOffset unwind shifts LegendList's draw window, so
estimate-only rows measure in at their real sizes mid-dismiss; LegendList's
JS MVCP compensates against that moving target and settles short of the end,
leaving the newest message below the fold. Re-pin frame-by-frame from
keyboardWillHide until shortly after keyboardDidHide when we were at the end
when the hide started.
@chrisnojima
chrisnojima force-pushed the nojima/HOTPOT-legends-310 branch from f20e0f9 to 8a016ed Compare August 10, 2026 22:29
Two defects kept a search hit from landing where it was asked to, one per
platform, both fixed in the library rather than worked around here.

On desktop the scroll extent legend-list clamps against is measured from the
DOM, which lags a React commit. The jump to a hit runs on the tick the thread
data changes, so the requested offset was clamped to a stale extent - usually 0 -
and the thread sat at the top. On native the scroll landed against estimated row
heights and nothing re-aimed once the rows above measured taller, so hits drifted
off screen.

With the library fixed, the elaborate app-side compensation is unnecessary: the
desktop centering loop, the native re-assert timers, the prepend guard and the
keyboard re-pin loop all come out, leaving a single scrollToIndex per target.

maintainVisibleContentPosition on native now stays mounted with one config
instead of being switched off while centered. It is what holds the hit in place
while the centered load streams older messages in, and toggling the prop makes
the list jump.

Patch regenerated from the fork build; both fixes are proposed upstream.
Review of the patch found three user-visible problems with the first cut, all
fixed upstream in the fork and regenerated here.

- The web fix gated every imperative scroll on the document extent matching the
  list's content size. Those disagree permanently whenever contentInset is in
  play, since web never applies the base inset to layout, so every scroll would
  have waited out the full 800ms readiness timeout. It is now a bounded re-issue
  inside the web scroll view, which also covers the initial scroll, scroll
  adjustments and completion checks rather than only the imperative path.
- Settling a scroll target cancelled on onScrollBeginDrag, which is not wired on
  web. Scrolling within a second of jumping to a search hit yanked the list back.
  Cancellation is now driven by a scroll event landing somewhere other than where
  it was asked to, which covers wheel, trackpad, scrollbar and keyboard.
- Settling armed for animated scrolls too, turning them into an instant jump, and
  corrected from inside the layout pass rather than after it.

Also bumps @legendapp/list to 3.3.5 and drops the stale 3.3.4 patch, matching
what #29526 did on master. The patch here targets 3.3.5, so without the bump
patch-package would silently skip it on a clean install.
Both fixes had blockers a review caught, so the patch is regenerated from the
reworked fork branches.

Web scroll re-issue:
- it kept scrolling after the list unmounted, which in window scroll mode meant a
  dead component scrolling the page for up to ten frames
- scrollToEnd never re-issued at all, because it asks for the extent it just
  measured and so was never out of reach - the one path that matters most when
  content has just grown
- retries now stop as soon as the extent stops growing rather than always burning
  the full frame budget, and interference is judged against where the scroll
  actually landed so a platform that clamps differently is not mistaken for the
  user

Scroll target settling:
- the user-scroll release was dead code: onScroll assigns state.scrollPending to
  the incoming offset immediately before calling updateScroll, so the comparison
  was always zero. Corrections no longer claim the scroll session, which lets the
  existing user-scroll detection do the work
- the correction budget was spent per layout pass rather than per correction, and
  layout runs many passes per frame while items measure, so on the very lists
  this targets it could spend the whole budget and issue nothing
- maintainScrollAtEnd now releases the target when it requests the end anchor
  rather than a frame later, so a queued correction cannot slip in first
Jumping to a hit, waiting, then scrolling pulled the list back to the hit.
Scrolling up is what asks for older messages, the prepend shifts every index, and
the centering effect treated a shifted index as a new target to scroll to - so
reading around a hit re-centered the list out from under you.

The index tracking was there because a prepend does move the target out from
under an already-issued scroll. That is now handled where it belongs: the list
holds an imperative scroll target in place while rows measure, and
maintainVisibleContentPosition holds it across prepends. So this scrolls once per
target on both platforms; desktop had the same flaw and popped back on
wheel-triggered pagination.

Also picks up the reworked legend-list patch: the web scroll fix now borrows the
room a request needs through bookkeeping shared with ScrollAdjust, rather than
gating scrolls on a readiness check that could stall them.
Verifying search-hit landing has meant rebuilding, reloading and stepping through
hits by hand, which is slow and easy to get wrong. This drives it: step past the
end of the hit list so the search wraps, jump to a hit already on screen, and
drag the thread after landing.

Asserting the hit is on screen has to compare where the row is against where the
list is. The row keeps its marker while it is the selected hit, but a virtualised
list also renders rows outside the viewport, so asserting the marker exists
passes against a build where the hit lands off screen - verified by disabling the
scroll-target settle and watching it stay green.

The marker sits on the row wrapper that already exists, so no extra view is
introduced. iOS 26 keeps the conversation's actions in a native overflow menu
with no React view to tag, so the flow addresses those by the accessibility
labels the platform exposes.

What each case is worth today, having mutated the code under each:

- wrapping around: real. Disabling the settle fails it with the row off screen.
- hit already on screen: passes, but no native mutation I tried made it fail -
  the fix it covers is on web. Kept as a guard, worth little on iOS.
- drag after a hit: no teeth yet. It survives disabling the settle, removing the
  measurement gate, removing the drag release, and restoring the index-shift
  re-issue that caused the reported snap-back. The reported failure needed the
  drag to page older messages in; three drags from where the hit lands does not
  get there, and by then the settle's deadline has passed. Needs to drive until a
  page-in actually happens.

Also picks up the reworked legend-list patch: the settle now keys on measurement
rather than on guessing who scrolled, and remembers a measurement seen while
another controller was compensating instead of dropping it - which is why hit 10
never re-aimed.
The case is "search, wait, then scroll away" - the failure people actually
reported. It only means anything if the thread loads another page while the
reader is dragging, since the prepend that follows is what used to re-centre the
list out from under them. The previous version dragged a fixed distance and
asserted the row had not moved much, which survived every mutation I tried.

Drive it instead: fling back through the thread until the top-of-thread marker
jumps thousands of pixels away, which is a prepend and nothing else, then assert
the hit has not come back on screen. If no page-in happens within the fling
budget, fail rather than pass - the case was never exercised, and saying so is
worth more than a green tick.

Verified by mutation, against a freshly bundled app: restoring the index-shift
re-centre in list-area fails this case three times over with "the hit never left
the viewport", and it passes without it.

Also updates the legend-list patch to the reworked fork branch: the padding side
for horizontal lists, a bound on how far a scroll may reach past the committed
content, a data change no longer counted as a measurement, and web
user-interaction detection that follows movement rather than contact.
Review of the first version turned up three ways it could pass or fail for the
wrong reason, all of them things that would only show up on a device that isn't
this one:

- `~More` and `~Search` matched anywhere in the tree, including the More tab and
  the inbox's own search field. Both are now scoped to the navigation bar and the
  presented menu.
- Gestures used a hardcoded x of 200, which on a tablet lands in the inbox pane
  beside the thread, so the thread would never page in and the failure would
  claim it did not load. Gestures are now measured from the thread's own frame,
  less the search bar that overlays it.
- Keys went to whatever had focus, but the search field focuses itself a beat
  after mounting. It carries a testID now and the flow types into it, which also
  routes through the paste path that exists because per-key injection crashes the
  app on the older sims.

The page-in detector watched for the string "Digging ancient messages...", which
unmounts in the very window it was watching for - the marker is replaced while
the load is in flight. SpecialTopMessage carries a testID now and is mounted in
every state, so the flow measures a thing that stays put.

The testID'd wrappers on the hit row, the search bar's Cancel and the search
input carry collapsable={false}, or Android view flattening leaves the testID on
an empty leaf - which on that platform would make the drag case pass vacuously,
since a missing row reads as "not on screen", which is what it asserts.

Assertions now report what they saw, require more than a hairline of the row to
be visible, read the hit count out of the search bar so wrapping around is
guaranteed rather than assumed, and stop swallowing driver errors as "off
screen".

On teeth, honestly: the wrapping case is real. The drag case reliably provokes a
page-in and logs it, but its snap-back detection is timing-dependent - the app
mutation that used to fail it three times out of three now fails it about one run
in three, and mocha's retries can hide even that. Do not read a green there as
proof. The same-screen case still has no native mutation that fails it.

Also updates the legend-list patch to the reviewed fork branch.
The rewrite in the previous commit lost the drag case's teeth: the app mutation
that used to fail it three times out of three failed it about one run in three.
Restoring the previous version of the file and running it against the current
code failed 3/3, which placed the fault in the test rather than in the library.

The cause was the order. The flow provoked a page-in and *then* dragged, so the
prepend - and the re-centre it triggers - usually landed while the reader was
still scrolling, where a thread that jumps back is indistinguishable from one
being flung. The reported failure is the other way round: search, stop, then
scroll, and the page that arrives underneath yanks the thread back.

So the reader moves off the hit first, then keeps reading back through older
messages until a page arrives, and the hit must not return at any point during
that or in the window after it settles. The final check also watches the row's
position rather than only its visibility, since whether the row can be *seen*
depends on the search bar and the keyboard, while whether the thread travelled
back toward it does not.

Verified both directions against a freshly bundled app: 3/3 failures with the
index-shift re-centre restored, reporting where the row and the thread were, and
a clean pass without it.
…times

Measured the drag case over four back-to-back runs on unmodified code: three
failed, all with the same message and the same geometry - "row 731..785, thread
116..785". Not flake. One 200px drag moves a centred hit to the bottom edge of
the viewport and no further, so whether the row cleared the edge came down to its
height and the screen's.

Worse, that is the same message the mutation runs produced, so the "3/3 detection"
in the previous commit was partly this defect firing rather than the regression
being caught. Both readings were wrong for the same reason.

Drag until the row is actually off screen, up to a bound. Then the state the rest
of the case depends on is a fact rather than a hope, and a thread that re-centres
itself still fails - because the drag can never get rid of the hit.

Measured after the change, freshly bundled each time:
- unmodified: 3 runs, 3 passing, no retries
- index-shift re-centre restored: 2 runs, both failing at "the thread scrolled
  back to the hit while reading", with the row and thread bounds in the message

Two runs of the suite fit in the time one used to take with its retries.
The suite retries twice by default to absorb load on a long single-session run.
That is defensible for flows where a failure means "the sim was busy", but this
one exists to catch a thread that scrolls itself, and a retry cannot tell that
apart from a slow simulator - it just runs again and reports green.

It hid a real 3-in-4 failure rate from me while I was measuring this flow: the
run that "passed" needed two retries to do it.

Measured stable without them across four runs on unmodified code, and it still
fails on the first attempt when the regression is present.
The config claimed "a real break fails all attempts". That is not true of every
flow: a chat-search regression that failed three runs in four was reported green,
because a retry cannot tell a slow simulator from a thread that scrolled itself.
It cost a full round of measurement to notice.

So the default is 0 on both iOS and Android, and a flow that is genuinely
load-sensitive asks for retries in its own describe, where the reason sits next
to the test it excuses.

Measured a full iPhone suite run to decide who gets one. Exactly two tests owed
their pass to a retry: people-profile's feed render, which waits on a network
load, and one visual-states case. people-profile keeps retries; visual-states
does not, because it is currently failing for an unrelated reason (below) and a
retry there would hide it rather than absorb it.

Unrelated finding, not addressed here: 20 of 23 visual-states tests fail, in
isolation as well as in the full suite, all at tab navigation ("testID
teams-list/files-browser/chat-message-list never appeared"). They cascade from
'new chat team builder', after which escapeToTabs cannot get the app out of the
New Chat modal. Not caused by the testIDs in this branch - it takes out Files,
Teams and Settings equally.
…efore

visual-states was failing 20 of 23 on this branch. Two independent holes in the
reset between tests, both of which also outlive a run, because the app restores
its last screen on launch and the after-hook uses the same reset.

The keyboard. WDA answers "Did not know how to dismiss the keyboard" for the chat
composer - no Done key, no accessory to press - so dismissKeyboard silently gave
up. That is not cosmetic: while the keyboard is up the screen's own controls stop
reporting as hittable, so the back chevron is invisible to tapNavBack and the
left-edge pop does not take. escapeToTabs then spent its whole budget in a
conversation it could not leave, ~50s per test, and every flow after it started
from the wrong screen. Instrumenting the exhaustion path is what found this: it
reported people=0, nav=[Back|chatConversation|More], and visible buttons that
were all keyboard keys. Now it blurs the composer by tapping the content above
it, the way a person would.

The modal. A modal presented over the tabs leaves the tab bar - and the whole
screen behind it - in the accessibility tree, so atTabs read "already at the
root" while a New chat or account-switcher modal was still up, and the reset
returned with it still there. The next flow then tapped what it thought were
inbox rows and got the modal's people list. Measured while the New chat sheet was
up: People present but not visible, five inbox rows, and a visible Cancel, all at
once. The reset now dismisses what is on top before asking whether it is home.

Both fixes are in the reset rather than in the tests, because this suite has
tests deliberately end on the state they capture - the after-hook comment says so
- which makes cleaning up the reset's job.

visual-states: 23 passing, three runs in a row, having been 3 passing / 20
failing. No retries anywhere.
people-profile matched its own username anywhere on screen. The People header's
avatar carries that username too, so the tap could land there instead of on a
feed row - which opens the account switcher, fails this test on a missing profile
page, and leaves a modal up for whatever runs next.

It had retries, so it had been passing on the second attempt and reading green.
With retries opt-in and this flow's reason for keeping them being network
slowness, it started failing all three attempts and showed what it was really
doing.

Scoped to the feed. Passes twice in a row now without consuming a retry.
…h hits

Validating on Electron caught a regression I introduced in the fork: searching a
thread scrolled nowhere, leaving the hit off screen with the list at the top -
the exact defect this whole effort set out to fix.

The cause was a ceiling I added on how far a scroll may reach past the committed
content, on review advice, to avoid materialising blank scrollable space for a
nonsense request. It refuses the case the feature exists for. Traced in the app:
offset 9047, committed DOM extent 0, totalSize-derived bound 8932 - so the
request was clamped to 0 and the list never moved. The offset is resolved from
positions that include estimates, so it can sit past the list's own totalSize
while the DOM reports nothing yet; neither number bounds it.

The ceiling is gone. Only a non-finite offset falls back to clamping now, and the
fork carries a test built from those measured numbers.

Verified after the change:
- Electron: 'working' lands with scrollTop == maxScroll and the row on screen; 21
  hits of 'one' stepped through the wrap-around, none off screen; scrolling away
  from a hit stays put.
- iOS: chat-search-hit 3/3, page-in provoked.

Worth recording: the desktop app imports @legendapp/list/react, and I spent an
hour instrumenting the react-native.web build before noticing.
…nt honestly

Four findings from a review of the retries and reset work, all cases where a test
could mislead rather than fail:

people-profile sent an iOS predicate through a scoped query, which uiautomator2
does not understand. On Android that throws into an isExisting() catch, the flow
returns early, and the test passes having asserted nothing - a permanent silent
green. There is now a byTextWithin helper with the same platform split byText
already had, escaping included. Its retries are gone too: the stated reason was a
slow feed, but nothing waited on feed content (the container mounts empty and
immediately), so slowness never failed - it bailed green. A bounded wait on the
row itself replaces two full re-runs, and says in the log when it bails.

dismissKeyboard tapped the content above the composer. The chat list sets
keyboardShouldPersistTaps="handled", so a tap landing on a row is handled by that
row - the keyboard stays up AND the row does whatever it does, which in this
account can mean opening an attachment or following a link out of the app. It
drags now, which is the gesture keyboardDismissMode="on-drag" listens for and
which cannot activate a touchable. It also logs when the keyboard survives,
rather than leaving escapeToTabs to burn its budget with nothing in the log.

The reset's dismiss-before-atTabs loop waited via settleAfter, which is defined in
terms of atTabs - the predicate that lies while a modal is up - so it returned
instantly and the loop gave up after one iteration. It waits on the control itself
now. It also took the first matching control, but a partly-covering sheet leaves
the background's controls earlier in the tree, so the first match can be clicked
through the sheet; it takes the last. And its predicate matched Done/Close/Cancel
as substrings, which would make a future "Close team" an unattended destructive
click in the reset - it requires an exact name on a button or menu item now.

chat-search-hit read "N of M" by matching " of " across the whole screen, with the
thread's own message text behind the bar and 'one' chosen because it is common. A
message body could win the match and either accuse the app of finding no hits or
silently stop the wrap-around from being exercised. The counter carries a testID
on both platforms now, read through the text node inside it, and says what it saw
when it cannot parse a count.

Also updates the legend-list patch: ScrollAdjust now measures the content without
room the scroll view is borrowing on the same node, and a settling target's
corrections no longer refresh the wheel-momentum grace that is meant to let a user
interrupt them.

iPhone suite: 61 passing, no retries consumed. chat-search-hit 3/3 on device with
the new counter path.
Review round two on the harness. Each of these could report success without it
being true:

The pre-loop waited for the dismissed control to disappear using isExisting,
which re-runs the selector rather than checking that element - so any other
Done/Close/Cancel on screen (the layer behind, a second sheet, the keyboard's own
toolbar) answered "still there" and broke the loop after one iteration, which is
the failure the loop was added to fix. It checks the element itself now, through
isDisplayed, and treats a stale reference as gone.

Its predicate excluded StaticText, which dropped the thread search bar's Cancel -
a Kb.Text with an onClick. On iPad that is the one control that matters: atTabs is
already true inside the Chat tab, so the loop below never runs and the pre-loop is
the only thing that can close a leaked search bar.

dismissKeyboard only knew how to dismiss the chat list's keyboard. The drag is
right there - keyboardDismissMode="on-drag", and a tap can be swallowed by
keyboardShouldPersistTaps="handled" - but no other screen sets either, so the
drag was a regression everywhere else. It now asks WDA to press a named key
first, then drags, then falls back to the tap at the height that used to work,
and each step is tried only while the keyboard is still up. Caught by the log
this added last round: "keyboard still up after drag and tap", on the team
wizard, the one screen whose keyboard the drag could not reach.

readHitCount could not tell "no results" from "could not read the counter", and
reported both as a search that found nothing - the misdiagnosis the testID was
meant to remove. Unreadable is its own answer now. It also read the count while
results were still streaming, so the step budget could be a partial number and
the wrap-around case would quietly stop wrapping; it waits for two reads to
agree.

byTextWithin escaped Android text with an ObjC-predicate escaper, which XPath
cannot read, and matched descendants only where byText matches the root too.

people-profile rebuilt its scoped query per poll: an element caches its parent's
id, so a feed that re-renders makes every later poll throw stale, which the catch
turned into a silent skip.

Also updates the legend-list patch: the web scroll view now reports user
interaction upward through a prop instead of importing core and clearing list
state, a scroll carries whether it is the list re-aiming itself, and the
momentum-grace sentinel starts at "never armed" rather than at time zero - which
had been swallowing every wheel for the first 150ms of a page's life.

iPhone suite: 61 passing.
The patch update carries four library fixes. The borrowed-room bookkeeping now
records the extent a borrow bought rather than the padding it asked for — the two
differ whenever the content box swallows part of a request — so the scroll event,
the committed-extent check and ScrollAdjust all subtract the same number. A
correction re-aims the pending scroll by ownership instead of by comparing
indices, which a prepend shifts out from under it. A plain jump to the last item
settles again, rather than being refused for an end alignment scrollToIndex filled
in on the caller's behalf when nothing is anchoring the end. And a second finger
landing mid-drag no longer re-anchors the slop origin.

On the app side, native passed dataKey={conversationIDKey}, so it never took the
reset desktop takes on clearVersion — and every centered load (search hit,
reply-quote jump, pinned message) clears the thread before refetching it. For the
same reason lastCenteredOrdinal is now reset per dataset: re-centering on the
ordinal already stored still reloads the thread, so the list has to be sent to it
again.

useKeyboardChatComposerInset was inert as wired — composerRef was never attached
and onComposerLayout was discarded, leaving contentInsetEndAdjustment pinned at 0,
which is what the prop defaults to anyway.
…ying

The desktop thread-search cases were manual: search a thread, step every hit, and
confirm the list is left where the reader drags it. They are a flow now. The
header's search icon carried a testID only on the mobile branch, so the desktop
one was unreachable; it has the same testID now. The click needs force — the
conversation header is inside the window's drag region, which makes playwright's
actionability check wait forever on a control that is perfectly clickable.

Two existing checks passed without checking anything. The snap-back assertion read
the hit's resting position right after a drag whose whole job is to push the row
out of view, so when the row was unmounted the travel check was skipped and the
test fell back to visibility alone — which is the half that already had a check. It
now treats the row coming back into the render window as the same failure.

And escapeToTabs matched XCUIElementTypeStaticText by exact name, which also
matches a chat message whose whole body is "Cancel" — with the suite parked in a
thread, before every test. The one StaticText that needed dismissing was the thread
search bar, which is now closed by its own testID instead.
Stepping every hit costs more the more hits there are, and the conversation the
suite runs against keeps gaining them — 70 by now. The walk ran past mocha's
per-test budget, and the timeout surfaced as "the hit landed and then drifted off
screen": a product failure that had not happened. The wrap-around is forced
directly now (next, from the hit the search lands on, jumps straight to the far
end of the thread, which is the longest jump the list is ever asked to make) and
the walk after it is bounded, saying out loud how much of the ring it skipped.

The same-screen query was a word out of the conversation's history, and it stopped
matching — which the flow reported as the hit row never rendering rather than as a
query with no results. It searches for a word out of the messages this suite itself
sends now, which is both guaranteed present and recent.
Both flows opened the first inbox row, and the inbox is ordered by recency — so
which conversation they searched depended on what had most recently received a
message, including the messages these suites send themselves. A run could search
one conversation and the next run another, which changes the hit count and which
words match at all. Today that surfaced as the wrap-around walk running past its
budget and as a query returning nothing, neither of which had anything to do with
the list.

They open the smoke user's own chat by name now. It always exists, its name is the
username the run was given, and it has the history these cases need.
The patch carried a third fix reporting web input events — wheel, pointer, touch,
key — as the user taking hold of the list, so that a scroll target still settling
would let go. Measured against a build without it, a drag issued immediately after
a jump was not corrected back in any of six rounds, on either platform. The native
half of that release was never in it: onScrollBeginDrag already clears the settle.

The two that remain were each shown to be load-bearing by removing them from a real
build. Without the web extent borrow a search hit lands at y=1771 in a viewport of
y=80..867; without the settle it lands at y=1524. Both deterministic.

Roughly 700 lines of library source and 1300 of tests gone, and the patch is about
1300 lines smaller.
…croll

Reaching a scroll offset the committed content cannot satisfy yet was done by
extending the content with temporary end padding, scrolling into the room that
bought, and giving it back once the scroll had happened — sharing the bookkeeping
with the scroll adjustment that pads the same node, measuring what each borrow
actually bought, and holding the room for the length of an animated scroll.

Re-issuing the scroll on later frames until the content commits does the same job.
Same defect, same coverage, four hundred fewer lines of library source: the
padding module, its shared accounting, the animated-scroll release with its
scrollend listener and watchdog, and the scroll-adjust changes that existed only to
share with it are all gone.

Verified the way the rest of this branch was: iPhone suite 61 passing, the desktop
search flow green twice over, and the re-issue proven load-bearing by a build with
it disabled, where a hit lands at y=1524 in a viewport of y=80..867.
Three reviewers went over the library fork and the app side asking for
simplification. Two library findings held up and are folded into the commits they
belong to.

The re-issue was scoped to every non-window scroll, so the thread's own page-down
— scrollToOffset(scroll + scrollLength) — asks past the end at the bottom of a
thread every time, and burned up to thirty frames of redundant scrollTo on top of
the reader. It now stops as soon as the extent stops growing. And ownsScrollingTo,
added earlier the same day, could only ever be true: its one caller sits thirteen
lines below the assignment it was checking.

The desktop flow's second half could assert nothing: when the drag pushed the hit
out of the render window both readings were null and neither branch fired, which
is the same hole the iOS flow was already fixed for. It falls back to the top of
the thread now and fails when neither can be read. Its on-screen check also
accepted a one-pixel overlap where iOS requires twenty-four.

Reported and deliberately not changed: closing the thread-search bar clears the
center, and the desktop cleared branch then scrolls to the newest message when the
thread contains it — which contradicts leaving the thread where the reader put it.
It predates this branch and changing it is a UX call.
Two holes the reviewers found, and the previous commit claimed these were fixed
when the edit had silently not applied.

The second half could assert nothing. The drag before it exists to push the hit
out of the viewport, and far enough out the row unmounts — at which point both
readings were null, neither branch fired, and the test went green having checked
nothing. That is the same vacuity the iOS flow was already fixed for. It falls
back to the top of the thread, which is mounted in every state, and fails when
neither can be read.

The on-screen check also passed for a row overlapping the viewport by a single
pixel. It requires the same 24px the iOS flow does.
Desktop and native ran the same algorithm twice, down to a six-line comment
duplicated near-verbatim, differing only in how they looked the ordinal up and
which scroll call they made. Both refs point at the same list type, and it exposes
scrollToItem, so the lookup and the index-vs-item split both go away.

That removes a real defect with them. Closing the thread-search bar clears the
centre, and the desktop copy read a cleared centre as "go to the newest message"
whenever the thread contained it — so a reader who searched, landed on a hit and
cancelled the bar got yanked to the bottom, which is the behaviour the flows exist
to forbid. Native never had that branch, and now neither does desktop.

Also gone: the native sentinel ordinal that existed so the value could be compared
against -1 instead of undefined, and a ref-plus-stable-closure pair whose comment
said it kept the calling effect from re-running on every centred-ordinal change —
that effect listed the ordinal in its own dependencies, so it re-ran regardless.

The library patch loses the settle's time to live, which was refreshed on every
pass that could act and so could only elapse on a pass that had already returned
without doing anything; the hard deadline is what ends an idle one.
Two library fixes, both found by clicking through conversations in the running
Electron app and reproduced with playwright over its CDP port. Each is content
growing after the list has already anchored to the end it could see.

The end anchor read its own scroll settling as the reader taking hold, and gave
up the end in the frame the content grew past it - measured at 432px short of the
newest message. The platform now records the offset it actually scrolled to and
the anchor only stands down for a position it did not put the list in.

Nothing followed the header's first measurement. SpecialTopMessage lays out taller
than the size it renders with, one frame after the initial scroll, which pushes
every message down by the difference - measured at 52px. maintainScrollAtEnd grew
a headerLayout trigger for it, which this list opts into. Enabling
maintainVisibleContentPosition's size anchoring instead was tried in a live build
and still failed 2 of 6 opens: that path declines while a scroll is in flight,
which is exactly when a header first measures.

Fork commits b2d666bc and 1a56778e, patch rebuilt from dist.
The regression this covers was reported by hand and would have gone on being
reported by hand: nothing in either suite read where a thread came to rest.

Two things give the flow teeth. Image responses are delayed, so rows grow after
the initial scroll rather than before it - on a warm disk cache the same sweep is
green with the bug in place. And retries are off for this flow: with the library
fix disabled the first attempt failed and the retry passed, so the suite's default
single retry would have hidden it. It also counts the threads whose content
exceeded their viewport and fails if that count is zero, rather than passing by
measuring nothing.

Big-team channel rows had no testID, so the conversations with enough history to
grow were unaddressable; they carry one now, next to the small-team row's.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant